1374717 - "Open default browser settings" in onboarding does nothing

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect, P1)

Description

[Jonathan Guillotte-Blouin [:jonathanGB]]

Running latest nightly on macOS. 

Other buttons like "Show private browsing in menu" work, only this one does not.

http://imgur.com/a/kCG9a

Comment 1

[Jonathan Guillotte-Blouin [:jonathanGB]]

NB: I have nightly as default already, but I feel like either we should:
1- not show the panel on "default" if it already is, OR
2- show some feedback saying "firefox is already your default browser" (e.g. some sort of feedback)

Comment 2

[[e-mail]]

Hi Michael, what we need to do if user have set Firefox as default browser before visiting the tour?


Maybe we can mark the tour as completed when we detected Firefox is already the default browser?

Comment 3

[Verdi [:verdi] Best to slack me]

(In reply to Fred Lin [:gasolin] from comment #2)
> Hi Michael, what we need to do if user have set Firefox as default browser
> before visiting the tour?
> 
> 
> Maybe we can mark the tour as completed when we detected Firefox is already
> the default browser?

I thought marking it as complete if it was already the default browser was cut for 57. Doing that would be nice if still possible. Whether or not we can do that, the button should still work. If Firefox is your default and you click it, it will just confirm that this is in fact the case. That's better than a button that doesn't work for an unknown reason.

Comment 4

[[e-mail]]

(In reply to Verdi [:verdi] from comment #3)
> (In reply to Fred Lin [:gasolin] from comment #2)
> > Hi Michael, what we need to do if user have set Firefox as default browser
> > before visiting the tour?
> > (del)
> Whether or not we can do that, the button should still work. If Firefox is your default and
> you click it, it will just confirm that this is in fact the case. That's
> better than a button that doesn't work for an unknown reason.

Ok, then what's the expect behavior when user click the button when he/she already set Firefox as the default browser?

Comment 5

[Verdi [:verdi] Best to slack me]

(In reply to Fred Lin [:gasolin] from comment #4)
> Ok, then what's the expect behavior when user click the button when he/she
> already set Firefox as the default browser?

The system dialog should open. In windows 10, for example, it will show that Firefox is set as the default browser already.

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Verdi [:verdi] from comment #5)
> (In reply to Fred Lin [:gasolin] from comment #4)
> > Ok, then what's the expect behavior when user click the button when he/she
> > already set Firefox as the default browser?
> 
> The system dialog should open. In windows 10, for example, it will show that
> Firefox is set as the default browser already.

That's not how any other OS works. Only Windows 10 asks the users to set default browser explictly in system settings. We still need to come up with something on other OSes.

Fred, could you make sure you get a implementable solution this week? Please also help Verdi understand the capablity of UITour; e.g. if we can dim the button or show a different message if we already know we are the default browser.

Comment 7

[[e-mail]]

Checked with verdi this afternoon and he suggest to 
1. disable the button and 
2. show a different message when we detected the user already set firefox as default. 

(According to the fact that we can detect the browser is set to default with UITour, but not easy to change the behavior with the system dialog, which the behavior detail is wrapped in platform API and may vary between OSes)


@verdi, could you help make the spec so we can start work on this?

Comment 8

[[e-mail]]

There are 3 cases need to be addressed in UI spec:

[not set to default]
When user open the default browser tour, the user will see the clickable button


[already set to default] 
When user open the default browser tour, the user will see the alternative description 
(ex: Nightly is currently your default browser)


[not set to default but already clicked the tour button (so the tour is marked as complete)] 
When user open the default browser tour, the user will see ?

Comment 9

[[e-mail]]

Since we like to decouple with the tour complete state (marking it as complete if it was already the default browser),
I suggest a new UX flow to solve this issue:

1. always show the button when user open the default browser tour
2. when user click the button, open the default browser settings (when firefox is not the default browser), or hide the button and show the alternative description at the same place(when firefox is the default browser)

With that interaction, the user can click only once and will solve the related issue (what should we do when user click the button several times).


Verdi, how do you think?


(un-assign myself since the UX of this issue is not final yet)

Comment 10

[[e-mail]]

Attachment: prototype

Hi Verdi, here's the prototype of above-mentioned proposal
Here's the video http://recordit.co/gRPNYUwRmg

1. When user click the button, it will disabled to prevent click again.
2. When user click the button and the browser already set to default, the button text will change to alternative string.

With this way we don't have to concern the tour complete state. Please let me know how you think.

Comment 11

[Verdi [:verdi] Best to slack me]

(In reply to Fred Lin [:gasolin] from comment #10)
> Created attachment 8884736 [details]
> prototype
> 
> Hi Verdi, here's the prototype of above-mentioned proposal
> Here's the video http://recordit.co/gRPNYUwRmg
> 
> 1. When user click the button, it will disabled to prevent click again.
> 2. When user click the button and the browser already set to default, the
> button text will change to alternative string.
> 
> With this way we don't have to concern the tour complete state. Please let
> me know how you think.

I talked with Michelle and the copy we'd like to use is, "You’ve got this! Firefox is already your default browser." Localization can ignore the "You've got this!" part if there isn't an equivalent. 

I don't think the alternate text should look like a button. I'll have to mock something up.

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Fred, is above actionable for you?

Comment 13

[[e-mail]]

Thanks Verdi,

If its not a button, what kind of element should I use? I could make a patch first then fix the style once you provided.

Comment 14

[[e-mail]]

And I'd like to make sure the prototype UI flow works for you before make a patch.

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Firefox 56 scope, need to get it done soon.

Comment 16

[Verdi [:verdi] Best to slack me]

Attachment: 5a-default-browser-overlay-set.png

Can we replace the button with two lines of body style, centered text.

Comment 17

[[e-mail]]

Yes, will do. Can you help do the the new string copy check to locale & legal team, so we can replace it at the time I land the patch.

Comment 18

[[e-mail]]

Attachment: Bug 1374717 - show alternative message when firefox already the default browser;

the call-to-action button will disabled when user click the button, to prevent open settings panel several times
the call-to-action button will be replaced to alternative text when Firefox is already the default browser

Review commit: https://reviewboard.mozilla.org/r/156856/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/156856/

Comment 19

[[e-mail]]

Comment on attachment 8886060 [details]
Bug 1374717 - show alternative message when firefox already the default browser;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156856/diff/1-2/

Comment 20

[[e-mail]]

In this patch:

* the call-to-action button will disabled when user click the button, to prevent open settings panel several times
* the call-to-action button will be replaced to alternative text when Firefox is already the default browser

Comment 21

[[e-mail]]

Comment on attachment 8886060 [details]
Bug 1374717 - show alternative message when firefox already the default browser;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156856/diff/2-3/

Comment 22

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Comment on attachment 8886060 [details]
Bug 1374717 - show alternative message when firefox already the default browser;

https://reviewboard.mozilla.org/r/156856/#review161966

Overall it looks good to me but I'm wondering the l10n part, would you add l10n member to look at it?

::: browser/extensions/onboarding/content/onboarding-tour-agent.js:24
(Diff revision 2)
> +      Mozilla.UITour.getConfiguration("appinfo", (config) => {
> +        let isDefaultBrowser = config.defaultBrowser;
> +        let btn = document.getElementById("onboarding-tour-default-browser-button");
> +        let msg = document.getElementById("onboarding-tour-is-default-browser-msg");
> +        if (isDefaultBrowser) {
> +          if (!btn.classList.contains("onboarding-hidden")) {

Maybe we can just call add/remove without if().

::: browser/extensions/onboarding/content/onboarding-tour-agent.js:27
(Diff revision 2)
> +        let msg = document.getElementById("onboarding-tour-is-default-browser-msg");
> +        if (isDefaultBrowser) {
> +          if (!btn.classList.contains("onboarding-hidden")) {
> +            btn.classList.add("onboarding-hidden");
> +          }
> +          if (msg.classList.contains("onboarding-hidden")) {

same above

::: browser/extensions/onboarding/content/onboarding-tour-agent.js:31
(Diff revision 2)
> +          }
> +          if (msg.classList.contains("onboarding-hidden")) {
> +            msg.classList.remove("onboarding-hidden");
> +          }
> +        } else {
> +          btn.disabled = true;

User may cancel the system dialog in case of mac, so maybe we don't have to disable it?

::: browser/extensions/onboarding/content/onboarding.js:180
(Diff revision 2)
>          <section class="onboarding-tour-content">
>            <img src="resource://onboarding/img/figure_default.svg" />
>          </section>
>          <aside class="onboarding-tour-button-container">
>            <button id="onboarding-tour-default-browser-button" class="onboarding-tour-action-button" data-l10n-id="${defaultBrowserButtonId}"></button>
> +          <div id="onboarding-tour-is-default-browser-msg" class="onboarding-hidden">${isDefaultMessage}<br/>${isDefault2ndMessage}</div>

Not sure if breaking this into two lines is okay for l10n, would you add l10n team member for review?

Comment 23

[[e-mail]]

Comment on attachment 8886060 [details]
Bug 1374717 - show alternative message when firefox already the default browser;

flod, could you take a look on l10n part?

Comment 24

[[e-mail]]

> ::: browser/extensions/onboarding/content/onboarding-tour-agent.js:31
> (Diff revision 2)
> > +          }
> > +          if (msg.classList.contains("onboarding-hidden")) {
> > +            msg.classList.remove("onboarding-hidden");
> > +          }
> > +        } else {
> > +          btn.disabled = true;
> 
> User may cancel the system dialog in case of mac, so maybe we don't have to
> disable it?
> 

I think to disable the button after user click still make sense.
If user want to trigger system panel again, user can open a new tab and the button will be recovered to the default state.

Comment 25

[Francesco Lodolo [:flod]]

Comment on attachment 8886060 [details]
Bug 1374717 - show alternative message when firefox already the default browser;

https://reviewboard.mozilla.org/r/156856/#review162060

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:59
(Diff revision 3)
>  onboarding.tour-default-browser.description2=Love %1$S? Set it as your default browser. Open a link from another application, and %1$S will be there for you.
>  # LOCALIZATION NOTE(onboarding.tour-default-browser.button): Label for a button to open the OS default browser settings where it's not possible to set the default browser directly. (OSX, Linux, Windows 8 and higher)
>  onboarding.tour-default-browser.button=Open Default Browser Settings
>  # LOCALIZATION NOTE(onboarding.tour-default-browser.win7.button): Label for a button to directly set the default browser (Windows 7). %S is brandShortName
>  onboarding.tour-default-browser.win7.button=Make %S Your Default Browser
> +# LOCALIZATION NOTE(onboarding.tour-default-browser.is-default.2nd-message): Label to admire user and show with onboarding.tour-default-browser.is-default.2nd-message.

Can you explain what you mean here? "admire" is not the verb you want.

I guess something along the line of "Label displayed when Firefox is already set as default browser, followed on a new line by "onboarding.tour-default-browser.is-default.2nd-message".

::: browser/extensions/onboarding/locales/en-US/onboarding.properties:61
(Diff revision 3)
>  onboarding.tour-default-browser.button=Open Default Browser Settings
>  # LOCALIZATION NOTE(onboarding.tour-default-browser.win7.button): Label for a button to directly set the default browser (Windows 7). %S is brandShortName
>  onboarding.tour-default-browser.win7.button=Make %S Your Default Browser
> +# LOCALIZATION NOTE(onboarding.tour-default-browser.is-default.2nd-message): Label to admire user and show with onboarding.tour-default-browser.is-default.2nd-message.
> +onboarding.tour-default-browser.is-default.message=You’ve got this!
> +# LOCALIZATION NOTE(onboarding.tour-default-browser.is-default.2nd-message): Label to show the browser is already set as the default browser. %S is brandShortName

Label displayed when Firefox is already set as default browser. %S is brandShortName

Comment 26

[Verdi [:verdi] Best to slack me]

(In reply to Fred Lin [:gasolin] from comment #17)
> Yes, will do. Can you help do the the new string copy check to locale &
> legal team, so we can replace it at the time I land the patch.

This copy is approved.

Comment 27

[[e-mail]]

Comment on attachment 8886060 [details]
Bug 1374717 - show alternative message when firefox already the default browser;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156856/diff/3-4/

Comment 28

[Francesco Lodolo [:flod]]

Comment on attachment 8886060 [details]
Bug 1374717 - show alternative message when firefox already the default browser;

https://reviewboard.mozilla.org/r/156856/#review162410

Comment 29

[Dave Townsend [:mossop]]

Comment on attachment 8886060 [details]
Bug 1374717 - show alternative message when firefox already the default browser;

https://reviewboard.mozilla.org/r/156856/#review162562

Comment 30

[[e-mail]]

thanks!

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f4aacb756bac
show alternative message when firefox already the default browser;r=flod,mossop

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f4aacb756bac

Comment 33

[Justin [:JW_SoftvisionQA]]

I have verified this is fixed on todays nightly across all platforms.

Comment 34

[Verdi [:verdi] Best to slack me]

Comment on attachment 8884736 [details]
prototype

Clearing review request

Comment 35

[Justin [:JW_SoftvisionQA]]

On Ubuntu no settings panel is shown. The default browser is automatically set to Nightly. Is this expected? 

If so, maybe change "Open default browser settings" to "Set Nightly as your default browser" on Ubuntu.

I can write a bug for this if you all agree.

Comment 36

[Verdi [:verdi] Best to slack me]

Ah yes, if this is the case with Ubuntu, it should use the same copy as Windows 7, "Make Firefox Your Default Browser".

Comment 37

[[e-mail]]

Sorry Justin I've reply this but not send:/

In short we should check other linux distribution as well (fedora, ubuntu with KDE) to make sure its ubuntu only or it applied to all linux distributions. Currently we don't have a way to identify ubuntu from other linux distribution.

Comment 38

[Matthew N. [:MattN]]

In case people didn't know, this is what we have the `canSetDefaultBrowserInBackground` property for on the getConfiguration("appInfo") response. Please update and use that rather than doing custom OS checks.